feat(ceiling): disjoint-split depth extrapolation (replaces bootstrap)#243
feat(ceiling): disjoint-split depth extrapolation (replaces bootstrap)#243beabevi wants to merge 3 commits into
Conversation
Replace the bootstrap self-split in compute_ceiling with a disjoint (without-replacement) split at multiple depths, extrapolated to full depth. The bootstrap drew both halves from the same cells, so they were not independent, which biased the ceiling in both directions (not a reliable upper bound): the shared cells make the halves over-agree (inflating it), while the duplicate cells over-call the FDR-gated DE metrics and drag the recovery metrics down. A disjoint split is unbiased but shallow (each half <= n/2); measuring each metric across depths and extrapolating to full depth corrects for it. For reliability-like (correlation) metrics this extrapolation reduces to the analytical Spearman-Brown correction (verified: extrap matches SB on those metrics); it generalizes the same idea to metrics with no closed form. - remove _bootstrap_halves; add _disjoint_halves + a per-metric depth-curve fit - compute_ceiling returns one per-metric table (was a (results, agg_results) tuple); writes a single ceiling_results.csv (was ceiling_results.csv + agg_ceiling_results.csv) - CLI --ceiling help, README, CLAUDE updated; add tests/test_ceiling_extrap.py
There was a problem hiding this comment.
Code Review
This pull request refactors the data ceiling estimation in cell-eval to use a disjoint-split depth extrapolation method instead of a bootstrap self-split, which avoids bias in the ceiling estimates. The changes update the CLI, documentation, MetricsEvaluator class, and unit tests. The review feedback highlights three opportunities to improve robustness: validating that the fracs parameter is not empty to prevent a crash in min(), guarding against empty index lists in np.concatenate during disjoint splitting, and filtering out floating-point NaN values during metric aggregation to prevent downstream extrapolation failures.
| fracs = tuple(sorted(set(fracs), reverse=True)) | ||
| if any(f <= 0.0 or f > 1.0 for f in fracs): | ||
| raise ValueError(f"fracs must be in (0, 1]; got {fracs}") |
There was a problem hiding this comment.
If fracs is empty, min(fracs) on line 206 will raise a ValueError: min() arg is an empty sequence. Explicitly validating that fracs is not empty prevents this crash.
| fracs = tuple(sorted(set(fracs), reverse=True)) | |
| if any(f <= 0.0 or f > 1.0 for f in fracs): | |
| raise ValueError(f"fracs must be in (0, 1]; got {fracs}") | |
| fracs = tuple(sorted(set(fracs), reverse=True)) | |
| if not fracs: | |
| raise ValueError("fracs must not be empty") | |
| if any(f <= 0.0 or f > 1.0 for f in fracs): | |
| raise ValueError(f"fracs must be in (0, 1]; got {fracs}") |
| # Disjoint split has no duplicate rows, so obs names stay unique. | ||
| half_real = real[np.concatenate(a_idx)].copy() | ||
| half_pred = real[np.concatenate(b_idx)].copy() |
There was a problem hiding this comment.
If no perturbations have enough cells to split at the given frac (e.g., if eligible is empty or cell counts are too low), a_idx and b_idx will be empty. Calling np.concatenate on an empty list raises a ValueError. Guarding against this with a descriptive error improves robustness.
# Disjoint split has no duplicate rows, so obs names stay unique.
if not a_idx:
raise ValueError(
f"No perturbations had enough cells to split at frac={frac}. "
"Ensure your dataset has sufficient cells per perturbation."
)
half_real = real[np.concatenate(a_idx)].copy()
half_pred = real[np.concatenate(b_idx)].copy()| arr = results[col].drop_nulls().to_numpy() | ||
| if arr.size == 0: | ||
| continue | ||
| out[col] = float(np.median(arr) if agg == "median" else np.mean(arr)) |
There was a problem hiding this comment.
Polars' drop_nulls() drops missing values (null), but does not drop floating-point NaN values. If any metric computation returns NaN for a perturbation, np.median or np.mean will propagate the NaN, causing the subsequent curve fitting/extrapolation to fail. Filtering out non-finite values (including NaN and inf) ensures robust aggregation.
| arr = results[col].drop_nulls().to_numpy() | |
| if arr.size == 0: | |
| continue | |
| out[col] = float(np.median(arr) if agg == "median" else np.mean(arr)) | |
| arr = results[col].drop_nulls().to_numpy() | |
| arr = arr[np.isfinite(arr)] | |
| if arr.size == 0: | |
| continue | |
| out[col] = float(np.median(arr) if agg == "median" else np.mean(arr)) |
|
can you bump the semver as this changes behavior: version = "0.8.1" -> version = "0.8.2" |
|
Incorporating the previously discussed plots for training on 0.25, 0.5, and 0.75 while predicting at 1.0 (reaching a 1.33x extrapolation), as well as training on 0.25 and 0.5 while predicting at 1.0 (reaching a 2.0x extrapolation). Additionally, including plots that use the mean instead of the median and illustrate the behavior across datasets and methods.
|
…rapolation) Replace the multi-depth reciprocal-fit extrapolation with a single disjoint self-split at n/2 corrected by the analytical Spearman-Brown doubling r' = 2r/(1+r). A holdout backtest across four datasets showed the plain Spearman-Brown correction is both the most accurate estimator and the cheapest (it needs only the deepest split), so the depth sweep and the free-intercept reciprocal fit are removed. - compute_ceiling: one _disjoint_halves(seed) split -> metrics -> Spearman-Brown - correction applied to an explicit SB_METRICS list of reliability metrics; error metrics, unbounded counts, and reliability metrics off the list (clustering_agreement, pearson_edistance) are emitted as NaN - returns (results, agg_results); writes ceiling_results.csv / agg_ceiling_results.csv, matching compute() - tests, README, and CLAUDE.md updated
be093ad to
245d2da
Compare
Switched the ceiling estimator to the plain Spearman-Brown correctionI've pushed a commit that replaces the multi-depth reciprocal-fit extrapolation with the analytical Spearman-Brown doubling Why. The holdout backtest — predict the held-out
What changed.
This builds directly on the disjoint-split groundwork in this PR — it just swaps the estimator that sits on top of it. @beabevi flagging since it's a change to your branch. |








Summary
Reworks the
--ceilingdata ceiling (MetricsEvaluator.compute_ceiling) from the bootstrap self-split to a disjoint-split depth extrapolation. For each perturbation the real data is split without replacement into two halves at several depths (fracs=(1.0, 0.5, 0.25)), the metric suite is run on each self-split, and every metric's value-vs-depth curve is extrapolated to full depth. The result is a per-metric, unbiased estimate of the best score any model could reach given the noise in the real data.Why
The bootstrap drew both halves from the same cells (
2ndraws with replacement, then split), so they weren't independent. That biases the ceiling in both directions, so it isn't a reliable upper bound:A disjoint (without-replacement) split gives genuinely independent halves (unbiased), at the cost of depth (each half ≤
n/2). Measuring each metric across several depths and extrapolating to full depth corrects for that depth cost.Relationship to Spearman-Brown: for reliability-like (correlation) metrics this extrapolation is the Spearman-Brown correction: SB (
2r/(1+r)) is its analytical closed form. The extrapolation generalizes the same idea to metrics that have no closed form (the DE recovery metrics), which is why it's used as the single method rather than SB.What changed
_bootstrap_halves; add_disjoint_halves(disjoint split at a depth fraction) + a per-metric depth-curve fit (attenuation/reliability form, linear fallback), extrapolated to full depth.compute_ceilingreturns one per-metric table (measure, withextrap_linear/resid/modeldiagnostics) instead of the old(results, agg_results)tuple.ceiling_results.csv(wasceiling_reslts.csv).--ceilingCLI help,README,CLAUDE.md). The existing ceiling tests intests/test_eval.pywere updated for the new API, and a newtests/test_ceiling_extrap.pywas added (unit tests for the extrapolation math + a disjoint-split check).pytest,ruff, andty(on the changed files) pass.